Review documentation and add BaseStationary to supplementary signs #167

carsten-kuebler · 2018-03-22T14:18:08Z

This PR addresses issue #97.

Regularize doxygen comments.
Add links to OSI message fields and enums.
Add "plural" form to repeated field names (add list/multiple, add _s etc.).
Remove typos and replace special chars.

Additional:

Split CanditateSupplementarySign in two messages (every supplementary sign has its own list of candidates and a base_rmse).
Add BaseStationary to SupplementarySign and remove its position_supplementary field.

Regularize doxygen comments. Add links to OSI message fields and enums. Add "plural" form to repeated field names (add _list/multiple_, add _s etc.). Remove typos and replace special chars. Split CanditateSupplementarySign in 2 messages (every supplementary sign has its own list of candidates and one base_rmse). Add BaseStationary to SupplementarySign and remove its position_supplementary field.

Typos removed again...

Fix doxygen links.

ghost

@carsten-kuebler I think we should be consistent with the plural forms.
value -> list -> list of lists
sensor_id -> sensor_ids -> question: sensor_ids_list ? or are is even a list of lists plural? What do you think?

Baseline: Definitly a perfect change to use plural form for repeated stuff:
Identifier sign
repeated identifier signs

carsten-kuebler · 2018-03-23T10:18:10Z

@CarloVanDriestenBMW

I would propose:

singular -> plural
lane -> lanes (Standard)
sensor_id -> sensor_id_list (ID is a abbreviation, so I would not add a plural s)
sensor_id_list -> multiple_sensor_id_lists (or sequence for "list of list")
point -> line (or polygon as an other term)

ghost · 2018-03-23T12:11:48Z

plural of abbreviations...hmmm true

I am in favor for: point -> points

It should be as simple as possible and also noted in the wiki how the rule is for OSI (guideline)

carsten-kuebler · 2018-03-23T12:44:59Z

point -> points.
So you would change
base_polyline -> base_polyline_points (or outline_points) and
center_line -> center_line_points

for BaseStationary and BaseMoving messages.

ghost · 2018-03-25T18:49:50Z

I actually forgot about the polyline...
I think there are two variants:

point -> points (they have no pattern or connection)
point -> polyline (the points together form a line, like a lane marking or a lane boundary?)

btw: sensor_id_list -> sensor_id_lists would be consistent, or?

ghost · 2018-03-26T06:52:35Z

osi_detectedlandmark.proto

+// \brief Candidates for all detected supplementary signs of one traffic sign
+// as estimated by the sensor.
+//
+message CandidateSupplementarySigns


@carsten-kuebler I think I do not understand this. Why is the base_rmse not in the "CandidateSupplementarySign". The name "candidate_multiple_supplementary_signs " seems not intuitive as well as I would not create two messages which sound so similar like "CandidateSupplementarySign" and "CandidateSupplementarySigns".
Should we have a call for that?

I agree, how would you call a list of supplementary signs -> sign forest or sign jungle. Both sounds colloquially.

We can have a call from 11:30. Is it ok?

carsten-kuebler · 2018-03-26T07:30:23Z

"btw: sensor_id_list -> sensor_id_lists would be consistent, or?"

I would say no.
It is a list of sensor IDs --> sensor_IDs (Capital letters should not be used)
Or a sensor ID list --> sensor_id_list
But not a list of list --> sensor_id_lists

ghost · 2018-03-26T11:17:20Z

Identifier sensor_id
repeated Identifier sensor_ids

// If new type necessary -> this would btw be in osi_common.proto
message IdentifierList
(
repeated Identifier sensor_ids
...
)

repeated IdentifierList sensor_id_lists

I think we can make it the same way with everything else eg. "detection", "detections" ....

carsten-kuebler · 2018-03-26T11:46:50Z

Identifier sensor_id --> repeated Identifier sensor_ids

OK!

// If new type necessary -> this would btw be in osi_common.proto
message IdentifierList
(
repeated Identifier sensor_ids
...
)

// If new type necessary -> this would btw be in osi_common.proto
message IdentifierList
(
repeated Identifier sensor_ids
)

No additional fields! If there are more fields (e.g. a header), then the message name should be different to "List"

I checked all _id_list. Everything would be ok, but what's about sensor_data_list --> there is no datas?!

Introduce new message MainSign. Messages for TrafficSign and DetectedXXX messages nested. Rename _id_list -> _ids. Rename fields and messages in FeatureData.proto. Restructure DetectedOccupant similar to DetectedLandmark.proto messages.

Remove plural s for repeated messages. Add note for documentation. Remove space at end of line. Remove typos.

carsten-kuebler · 2018-03-26T17:11:05Z

@CarloVanDriestenBMW I made the changes (also removed the plural s field names). All messages in osi_datarecording.proto are necessary.)

Missing postfix _id added.

ghost

now everything went wrong....

ghost · 2018-03-27T07:04:25Z

osi_datarecording.proto

    // List of sensor data for multiple sensors at a specific timestep.
    //
-    repeated SensorData sensors = 1;
+    // \note OSI uses singular instead of plural for repeated field names.


This is meant for the word "data" but not for everything else I guess?

Maybe I misunderstood the discussion with Pierre?!
So we use definitive PLURAL? --> No problem! All field names (which have to change are marked with the same note).

ghost · 2018-03-27T07:05:05Z

osi_datarecording.proto

-    repeated SensorData sensors = 1;
+    // \note OSI uses singular instead of plural for repeated field names.
+    //
+    repeated SensorData sensor = 1;


this should be sensor_data

ghost · 2018-03-27T07:06:31Z

osi_datarecording.proto

-    repeated SensorDataSeries sensors = 1;
+    // \note OSI uses singular instead of plural for repeated field names.
+    //
+    repeated SensorDataSeries sensor = 1;


sensor_data_series

The word series is both singular and plural. Same case as "data"

Please note that you would adress
sensor_data_series (SensorDataSeriesList) --> sensor_data (SensorDataSeries) -> ...

often you find in OSI like:
sensor(s) (you get a message SensorDataSeries) -> data (you get a message SensorData) -> ...

ghost · 2018-03-27T07:13:25Z

osi_detectedlandmark.proto

+    //
+    // \note OSI uses singular instead of plural for repeated field names.
+    //
+    repeated EstimatedSupplementarySign supplementary_sign = 5;


We have never used the word "estimated" before. It should be "detected"

We get a problem, if we use the message structure for the detected messages.

Would you prefer DetectedSupplementarySignData?

ghost · 2018-03-27T07:13:52Z

osi_detectedlandmark.proto

-
-    // The root mean squared error of the base parameters of the detected
-    // traffic sign.
+    // \note OSI uses singular instead of plural for repeated field names.


When did we say that???

see first comment

ghost · 2018-03-27T07:15:12Z

osi_detectedlandmark.proto

+        //
+        // \note OSI uses singular instead of plural for repeated field names.
+        //
+        repeated CandidateSign candidate = 1;


candidate_signs,

I have no idea why we lost the complete naming convention now...

Do you really think so. See some lines below. If we change consequent, then you must rename geometry --> traffic_sign_geometry...

ghost · 2018-03-27T07:16:38Z

osi_detectedlandmark.proto

+        //
+        // \note OSI uses singular instead of plural for repeated field names.
+        //
+        repeated CandidateSupplementarySign candidate = 1;


candidate_supplementary_signs

see previous comment.

ghost · 2018-03-27T07:17:34Z

osi_detectedlandmark.proto

+            // The definition of one of more supplementary signs that together
+            // define this candidate.
+            //
+            optional TrafficSign.SupplementarySign sign = 1;


supplementary_sign

ghost · 2018-03-27T07:20:26Z

osi_detectedlandmark.proto

+        //
+        // \note OSI uses singular instead of plural for repeated field names.
+        //
+        repeated CandidateTrafficLight candidate = 1;


We cannot do:
optional TrafficLight traffic_light = 1;
and at the same time:
repeated CandidateTrafficLight candidate = 1;

The naming convention must always follow the same pattern!
repeated CandidateTrafficLight candidate_traffic_lights = 1;

If the plural forms should be done in another PR we should at least use the proper singular names...

One option for a pattern is, to use the message name as field name. (drawback: if you use a message twice in a message the field names would be the same. There are other drawbacks..).

Second option: you choose a field name which is clearly in the message context and you would not repeat infromation several times.

In OSI we have a mix of both...

ghost · 2018-03-27T10:27:44Z

@carsten-kuebler ok, let's resolve the conflict and merge. We will do this with every PR and afterwards we go over everything again for naming and index numbers

carsten-kuebler · 2018-03-27T10:58:03Z

Ok. Should we synchronize our activities by phone?

ghost · 2018-03-27T13:25:51Z

right_lane_boundary_id

The "right adjacent lane boundaries" may only be shared with the "left adjacent lane boundaries" of the nearest "right adjacent lane"

And documentation review.

ghost · 2018-03-27T16:33:13Z

@carsten-kuebler please check again. I think I solved it...

carsten-kuebler · 2018-03-27T18:12:19Z

I reviewed the merge.

carsten-kuebler · 2018-03-27T18:18:45Z

Removed tabs.
Merge @CarloVanDriestenBMW update

carsten-kuebler · 2018-03-27T18:29:23Z

@CarloVanDriestenBMW PR is ok

…tionInterface/open-simulation-interface into Documentation-Review3

ghost

good job

carsten-kuebler added 6 commits March 22, 2018 14:53

Regularized the Doxygen comments.

84fa546

Regularized the Doxygen comments.

c4e4fec

Regularized the Doxygen comments.

cdc331c

Regularized the Doxygen comments.

caa1403

Bugfix

3f4abab

Typos removed again...

carsten-kuebler requested a review from a user March 22, 2018 16:42

carsten-kuebler added the Documentation Everything which impacts the quality of the documentation and guidelines. label Mar 22, 2018

carsten-kuebler added this to the v3.0.0 milestone Mar 22, 2018

carsten-kuebler added 5 commits March 22, 2018 17:46

Fix links.

a2051e2

Fix doxygen links.

Regularized the Doxygen comments.

86c57f3

Regularized the Doxygen comments.

5383b02

Fix doxygen links.

47e3445

Regularized the Doxygen comments.

b0afb29

ghost reviewed Mar 23, 2018

View reviewed changes

SebastianSchmidtBMW approved these changes Mar 23, 2018

View reviewed changes

carsten-kuebler and others added 3 commits March 23, 2018 15:42

Merge branch 'master' into Documentation-Review3

98db423

Removed typo.

ca26298

Correct Documentation

e6b0e76

for BaseStationary and BaseMoving messages.

carsten-kuebler mentioned this pull request Mar 23, 2018

Reference coordinate system for BaseStationary #173

Closed

ghost suggested changes Mar 26, 2018

View reviewed changes

carsten-kuebler added 2 commits March 26, 2018 12:58

Naming conventions

1335a83

Duplicated MeasurementState removed

1e0ba34

carsten-kuebler added 2 commits March 26, 2018 18:21

Review

74dd76d

Introduce new message MainSign. Messages for TrafficSign and DetectedXXX messages nested. Rename _id_list -> _ids. Rename fields and messages in FeatureData.proto. Restructure DetectedOccupant similar to DetectedLandmark.proto messages.

Review

5d09b97

Remove plural s for repeated messages. Add note for documentation. Remove space at end of line. Remove typos.

All Identifier field names with postfix _id

8c7c946

Missing postfix _id added.

ghost suggested changes Mar 27, 2018

View reviewed changes

Rename EstimatedXXX messages to DetectedXXXData.

d02b5a1

carsten-kuebler and others added 3 commits March 27, 2018 17:07

More DetectedXXX messages unified

672deb8

And documentation review.

fixed merge conflicts with master

e33ab45

Merge branch 'master' into Documentation-Review3

2e61c54

jdsika and others added 4 commits March 27, 2018 18:37

removed tabs in osi_sensordata.proto

179d4d8

fixed merge conflict in osi_sensorview.proto

70d5b01

Fixed enum style, unknow=, other=, etc,

56d0c15

Reviewed Merge

5d7725d

carsten-kuebler added 2 commits March 27, 2018 20:15

Review...

a4fe010

Reviewed...

27c9921

jdsika and others added 2 commits March 27, 2018 20:21

checked for plural forms and added notice for singular usage

e519f52

Reviewed

8fc2004

jdsika and others added 3 commits March 27, 2018 20:30

removed tab and resolved merge conflict

6a6584c

Merge branch 'Documentation-Review3' of https://github.com/OpenSimula…

d58011a

…tionInterface/open-simulation-interface into Documentation-Review3

Spaces and Tabs removed

7327dd7

ghost approved these changes Mar 27, 2018

View reviewed changes

ghost merged commit eb11168 into master Mar 27, 2018

ghost deleted the Documentation-Review3 branch March 27, 2018 18:43

This pull request was closed.

Review documentation and add BaseStationary to supplementary signs #167

Review documentation and add BaseStationary to supplementary signs #167

Uh oh!

Conversation

carsten-kuebler commented Mar 22, 2018

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

carsten-kuebler commented Mar 23, 2018

Uh oh!

ghost commented Mar 23, 2018

Uh oh!

carsten-kuebler commented Mar 23, 2018

Uh oh!

ghost commented Mar 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carsten-kuebler commented Mar 26, 2018

Uh oh!

ghost commented Mar 26, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carsten-kuebler commented Mar 26, 2018

Uh oh!

carsten-kuebler commented Mar 26, 2018

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 27, 2018

Uh oh!

carsten-kuebler commented Mar 27, 2018

Uh oh!

ghost commented Mar 27, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 27, 2018

Uh oh!

carsten-kuebler commented Mar 27, 2018

Uh oh!

ghost commented Mar 26, 2018 •

edited by ghost

Loading

ghost commented Mar 27, 2018 •

edited by ghost

Loading